-
Notifications
You must be signed in to change notification settings - Fork 352
[BoundsSafety][LLDB] Implement instrumentation plugin for -fbounds-safety soft traps #11835
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: stable/21.x
Are you sure you want to change the base?
[BoundsSafety][LLDB] Implement instrumentation plugin for -fbounds-safety soft traps #11835
Conversation
This refactors the soft trap runtime test so the soft trap runtime implementation exists in its own file. This has several advantages: * It let's the runtime be built without debug info which will be the common case uses hit * It prevents the risk of infinite recursion because it isn't safe to build the soft trap runtime with -fbounds-safety soft trap mode enabled.
f7206d3 to
a15e413
Compare
|
@swift-ci test |
|
@swift-ci test llvm |
…fety soft traps This patch implements an instrumentation plugin for the `-fbounds-safety` soft trap mode first implemented in swiftlang#11645 (rdar://158088757). The current implementation of -fbounds-safety traps works by emitting calls to runtime functions intended to log the occurrence of a soft trap. While the user could just set a breakpoint of these functions the instrumentation plugin sets it automatically and provides several additional features: When debug info is available: * It adjusts the stop reason to be the reason for trapping. This is extracted from the artificial frame in the debug info (similar to -fbounds-safety hard traps). * It adjusts the selected frame to be the frame where the soft trap occurred. When debug info is not available: * For the `call-with-str` soft trap mode the soft trap reason is read from the first argument register. * For the `call-minimal` soft trap mode the stop reason is adjusted to note its a bounds check failure but does not give further information because none is available. * In this situation the selected frame is not adjusted because in this mode the user will be looking at assembly and adjusting the frame makes things confusing. This patch includes shell and api tests. The shell tests seemed like the best way to test behavior when debug info is missing because those tests make it easy to disable building with debug info completely. rdar://163230807
a15e413 to
c8af09b
Compare
|
@swift-ci test |
|
@swift-ci test llvm |
|
Windows failure looks completely unrelated to this PR: likely caused by swiftlang/swift-docc#1331. Looks like swiftlang/swift-docc#1352 is out to revert this change. |
|
@swift-ci please test windows platform |
| case ArchSpec::eCore_x86_32_i686: | ||
| // Technically some x86 calling conventions do use a register for | ||
| // passing the first argument but let's ignore that for now. | ||
| return {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be worth explaining here that you can't get the information for this architecture? Otherwise the person receiving this report might waste time trying to figure out why this wasn't working in their particular case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If LLDB has a clean way of communicating this to the user I'm willing to use it (e.g. some kind of diagnostic system). At the moment is seems like the only thing I can do is shoehorn the failure into the trap reason message and I think that would be confusing. E.g.:
stop reason = Soft Bounds check failed: Cannot determine trap reason on x86 architecture when debug info is missing
The Cannot determ... part of the message is supposed to be read as the specific reason for trapping normally. Reusing it here to explain why the trap reason couldn't be determined seems a bit confusing.
Ideally what is something like:
stop reason = Soft Bounds check failed
note: Cannot determine trap reason on x86 architecture when debug info is missing
but I don't think anything like this exists today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can write warning messages directly by writing to the stream returned by Debugger::GetAsyncErrorStream, or you can post a warning event using Debugger::ReportWarning. I think posting a warning event is currently in favor.
| auto process = thread_sp->GetProcess(); | ||
| if (!process) | ||
| return {}; | ||
| switch (process->GetTarget().GetArchitecture().GetCore()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure you need to do this for this commit, but it would be clearer if the architecture told you whether it uses registers for argument passing, than having to hard code it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can file a github issue for this. Should this go to the upstream LLVM issue tracker or the Swift issue tracker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
llvm issue tracker, this is architecture and not swift specific.
| // Examine the register for the first argument | ||
| auto *arg0_info = rc->GetRegisterInfo( | ||
| lldb::RegisterKind::eRegisterKindGeneric, LLDB_REGNUM_GENERIC_ARG1); | ||
| if (!arg0_info) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Along this process there are a bunch of places where it could fail. It might help your sanity later on if you logged where the failure occurs. Then when this is failing on a machine or for a process you can't get access to you have more of a chance of figuring out what went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What channel should I use? Presumably you mean logging code like this (first thing I found when greping for LLDB_LOG) ?
Log *log = GetLog(LLDBLog::API);
LLDB_LOGF(log,
"SBDebugger(%p)::CreateTarget (filename=\"%s\", triple=%s, "
"platform_name=%s, add_dependent_modules=%u, error=%s) => "
"SBTarget(%p)",
static_cast<void *>(m_opaque_sp.get()), filename, target_triple,
platform_name, add_dependent_modules, sb_error.GetCString(),
static_cast<void *>(target_sp.get()));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL, there isn't an instrumentation runtime log channel, which there really should be. So maybe better to do this as a follow-on.
| return lldb::eInstrumentationRuntimeTypeBoundsSafety; | ||
| } | ||
|
|
||
| const RegularExpression & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Julian just added a InstrumentationRuntime::MatchAllModules. You still need to override this function but if you also override MatchAllModules to return true, we won't waste time pointlessly trying an empty regex against all the libraries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The override for MatchAllModules is declared in the header for InstrumentationRuntimeBoundsSafety
| return lldb::eInstrumentationRuntimeTypeBoundsSafety; | ||
| } | ||
|
|
||
| const RegularExpression & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The override for MatchAllModules is declared in the header for InstrumentationRuntimeBoundsSafety
| lldb::user_id_t break_id, | ||
| lldb::user_id_t break_loc_id); | ||
|
|
||
| bool MatchAllModules() override { return true; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jimingham The override is here. That should be sufficient right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, missed it. Yes, that's good.
| // Examine the register for the first argument | ||
| auto *arg0_info = rc->GetRegisterInfo( | ||
| lldb::RegisterKind::eRegisterKindGeneric, LLDB_REGNUM_GENERIC_ARG1); | ||
| if (!arg0_info) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What channel should I use? Presumably you mean logging code like this (first thing I found when greping for LLDB_LOG) ?
Log *log = GetLog(LLDBLog::API);
LLDB_LOGF(log,
"SBDebugger(%p)::CreateTarget (filename=\"%s\", triple=%s, "
"platform_name=%s, add_dependent_modules=%u, error=%s) => "
"SBTarget(%p)",
static_cast<void *>(m_opaque_sp.get()), filename, target_triple,
platform_name, add_dependent_modules, sb_error.GetCString(),
static_cast<void *>(target_sp.get()));
| auto process = thread_sp->GetProcess(); | ||
| if (!process) | ||
| return {}; | ||
| switch (process->GetTarget().GetArchitecture().GetCore()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can file a github issue for this. Should this go to the upstream LLVM issue tracker or the Swift issue tracker?
| case ArchSpec::eCore_x86_32_i686: | ||
| // Technically some x86 calling conventions do use a register for | ||
| // passing the first argument but let's ignore that for now. | ||
| return {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If LLDB has a clean way of communicating this to the user I'm willing to use it (e.g. some kind of diagnostic system). At the moment is seems like the only thing I can do is shoehorn the failure into the trap reason message and I think that would be confusing. E.g.:
stop reason = Soft Bounds check failed: Cannot determine trap reason on x86 architecture when debug info is missing
The Cannot determ... part of the message is supposed to be read as the specific reason for trapping normally. Reusing it here to explain why the trap reason couldn't be determined seems a bit confusing.
Ideally what is something like:
stop reason = Soft Bounds check failed
note: Cannot determine trap reason on x86 architecture when debug info is missing
but I don't think anything like this exists today.
lldb/source/Plugins/InstrumentationRuntime/BoundsSafety/InstrumentationRuntimeBoundsSafety.cpp
Show resolved
Hide resolved
|
|
||
| add_subdirectory(ASan) | ||
| add_subdirectory(ASanLibsanitizers) | ||
| # TO_UPSTREAM(BoundsSafety) ON |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a convention we follow for -fbounds-safety code. It's mostly useful for people resolving merge conflicts so they can clearly see what the code causing the conflict is for.
If you would prefer to not have it I can remove it but personally I find is useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this context it just leaves me confused why BoundsSafety is special in this regard? Why isn't this around other InstrumentationRuntimes, what does it do, etc.
But also, I just did
find . -type f -exec grep TO_UPSTREAM {} +
at the top-level llvm-project directory and got no hits. This certainly shouldn't be the first - unexplained - instance of it.
This patch implements an instrumentation plugin for the
-fbounds-safetysoft trap mode first implemented in#11645 (rdar://158088757).
The current implementation of -fbounds-safety traps works by emitting
calls to runtime functions intended to log the occurrence of a soft trap.
While the user could just set a breakpoint of these functions the
instrumentation plugin sets it automatically and provides several
additional features:
When debug info is available:
extracted from the artificial frame in the debug info (similar to
-fbounds-safety hard traps).
occurred.
When debug info is not available:
call-with-strsoft trap mode the soft trap reason isread from the first argument register.
call-minimalsoft trap mode the stop reason is adjustedto note its a bounds check failure but does not give further
information because none is available.
this mode the user will be looking at assembly and adjusting the
frame makes things confusing.
This patch includes shell and api tests. The shell tests seemed like the
best way to test behavior when debug info is missing because those tests
make it easy to disable building with debug info completely.
rdar://163230807